feat: add renderer diagnostics observability#392
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a renderer diagnostics subsystem: in-renderer emitters, incident detection, timeline/scroll/perf instrumentation, preload+IPC hooks, a main-process sanitizer/recorder/slicer/export, session-export & problem-report integration, menu/export wiring, and tests including an E2E probe. ChangesRenderer diagnostics end-to-end
Sequence Diagram(s)sequenceDiagram
participant Renderer as Browser Renderer
participant Emitter as Renderer Emitter
participant Preload as Preload API (window.api)
participant IPC as IPC Main
participant Recorder as Diagnostics Recorder
participant File as Local JSONL Storage
participant Report as Problem Report Builder
Renderer->>Emitter: emit(session.timeline.mount / scroll.sample / perf.sample / action.submit)
Emitter->>Emitter: normalize(monotonic_ms), detect incidents
Emitter->>Preload: window.api.emitRendererDiagnostic(event)
Preload->>IPC: invoke "renderer-diagnostics:record" (includes windowID)
IPC->>Recorder: record(sanitized event)
Recorder->>File: append JSONL line (serialize + retention flush)
Recorder-->>Recorder: rate-limit / cap by bytes / retention
Note over Report,IPC: During session export or problem report
Report->>IPC: request renderer diagnostics slice (sessionID, windowID, maxBytes)
IPC->>Recorder: slice(...)
Recorder->>Report: RendererDiagnosticsSlice (ok/truncated/missing/expired)
Report->>File: include slice in payload and save report
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). Review rate limit: 7/10 reviews remaining, refill in 12 minutes and 21 seconds. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive renderer diagnostics system for the desktop application, enabling the capture and recording of performance metrics, scroll behavior, and session state transitions. The implementation includes a main-process recorder that persists events to a JSONL log with retention and byte-capping logic, alongside a renderer-side emitter that integrates with SolidJS components. Feedback focuses on critical performance issues in the log pruning logic, specifically the
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/desktop-electron/src/main/problem-report.ts (1)
491-499:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate the new truncation field in
isTruncation().
parseProblemReportPayload()can now return aPayloadwhosetruncation.omittedRendererDiagnosticsBytesis missing or non-numeric, because the guard never checks it.Suggested fix
function isTruncation(value: unknown): value is Payload["truncation"] { if (!isRecord(value)) return false return ( isFiniteNumber(value.omittedMessages) && isFiniteNumber(value.omittedLogBytes) && isFiniteNumber(value.omittedSessionInfoBytes) && isFiniteNumber(value.omittedFailedExportErrorBytes) && + isFiniteNumber(value.omittedRendererDiagnosticsBytes) && isFiniteNumber(value.omittedDiagnosticsBytes) ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-electron/src/main/problem-report.ts` around lines 491 - 499, The type guard isTruncation currently omits checking omittedRendererDiagnosticsBytes, so update isTruncation (the function checking Payload["truncation"]) to also require isFiniteNumber(value.omittedRendererDiagnosticsBytes); ensure the guard returns false if that property is missing or non-numeric by adding it alongside the other isFiniteNumber checks so parseProblemReportPayload can't produce a Payload with an invalid truncation field.
🧹 Nitpick comments (2)
packages/desktop-electron/src/main/ipc-window-config.test.ts (1)
20-26: ⚡ Quick winAvoid locking this test to
src/main/ipc.tsas the wiring anchor.This test currently enshrines string-based channel registration inside
ipc.ts, which makes future migration to module-levelregister*Ipc()bootstrap wiring harder. Prefer asserting the bootstrap integration point instead of file-content substrings.Based on learnings: In Astro-Han/pawwork (packages/desktop-electron/src/main/), the bootstrap entry (packages/desktop-electron/src/main/index.ts) should directly call each module’s exported register*Ipc() function and should not centralize sub-module IPC registrations through src/main/ipc.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-electron/src/main/ipc-window-config.test.ts` around lines 20 - 26, The test is brittle because it asserts string substrings from src/main/ipc.ts instead of verifying the bootstrap wiring; update packages/desktop-electron/src/main/ipc-window-config.test.ts to assert that the bootstrap entry (src/main/index.ts) invokes the module-level registration function(s) (e.g., registerRendererDiagnosticsIpc or the generic register*Ipc exports) rather than checking for specific channel strings; modify the test to import the bootstrap (index.ts) and spy/mock the exported register*Ipc function(s) from the diagnostics module (or assert that index.ts imports and calls registerRendererDiagnosticsIpc) so the test validates integration at the bootstrap level and not file content.packages/app/src/app.tsx (1)
50-51: ⚡ Quick winAlign
Window.apityping with diagnostics export surface.Line 91 adds
emitRendererDiagnostic, but the pairedexportDiagnosticsLogmethod (already exposed on preload API) is missing from this interface. Adding it here keeps app-side typing consistent and avoids drift.♻️ Suggested typing alignment
-import type { RendererDiagnosticInput } from "@/context/platform" +import type { RendererDiagnosticInput, RendererDiagnosticsExportResult } from "@/context/platform" ... api?: { setDesktopContext?: (context: DesktopContext) => Promise<void> emitRendererDiagnostic?: (event: RendererDiagnosticInput) => Promise<void> + exportDiagnosticsLog?: () => Promise<RendererDiagnosticsExportResult> getAboutInfo?: () => Promise<AboutInfo>Also applies to: 89-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/app.tsx` around lines 50 - 51, The Window.api type is missing the exportDiagnosticsLog method causing app-side typing drift; update the Window.api interface (the same area that currently declares emitRendererDiagnostic) to also include exportDiagnosticsLog with the same signature as the preload API—use the existing RendererDiagnosticInput type if appropriate and ensure the method name and parameter/return types match the exported preload surface so both emitRendererDiagnostic and exportDiagnosticsLog are declared consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/e2e/session/session-renderer-diagnostics.spec.ts`:
- Around line 39-44: Replace the outright replacement of
win.api.emitRendererDiagnostic with a wrapper that delegates to the existing
implementation: capture the original function (e.g., const originalEmit =
win.api?.emitRendererDiagnostic), then assign a new async emitRendererDiagnostic
that calls await originalEmit?.call(win.api, event) (if present) and still
pushes a deep-cloned event into win.__pawwork_renderer_diagnostics; keep other
win.api properties untouched and preserve async/return behavior so a broken
preload/IPC handler is still exercised by the test while recording the event.
In `@packages/app/src/components/prompt-input/submit.ts`:
- Around line 523-540: The diagnostics are reading mutable state after calling
removeCommentItems() and clearInput(), and the fire-and-forget
emitRendererDiagnostic(...) silently drops rejections; capture metrics into
local variables (e.g., const commentCount = input.commentCount(), promptLen =
input.promptLength(currentPrompt), imageCount = images.length) before calling
removeCommentItems() / clearInput(), then call emitRendererDiagnostic with those
captured values and handle async failures (await the promise and wrap in
try/catch or attach a .catch to log/report errors) so failures aren’t ignored;
update references to messageID, session.id, model.providerID/model.modelID as
before when building the payload.
In `@packages/app/src/context/renderer-diagnostics.ts`:
- Around line 169-185: The emitted fps should be the real frames-per-second over
the sample period, not raw frameCount; in flush() replace fps: frameCount with a
computed value framesPerSecond = Math.round(frameCount / (sampleDurationMs /
1000)), where sampleDurationMs is the elapsed milliseconds since the previous
flush (track a lastFlushTimestamp set when diagnostics start and update it at
the end of flush(), initialize it on start); use framesPerSecond in the event
payload (and keep rounding/undefined handling consistent with other metrics).
In `@packages/app/src/pages/session/message-timeline.tsx`:
- Around line 739-755: The scroll diagnostic currently calls
emitRendererDiagnostic on every scroll event (hot path), causing excessive IPC
and jank; change it to throttle via requestAnimationFrame (or an RAF-backed
boolean guard) so emitRendererDiagnostic(...) runs at most once per animation
frame instead of per event, and clamp the computed distance_from_bottom (max -
el.scrollTop) to a non-negative value before including it in data; update the
same call site that uses emitRendererDiagnostic, visibleRangeData(),
props.hasScrollGesture(), props.scroll, and staging.isStaging() to use the
RAF-throttled emitter and the clamped distance metric.
In `@packages/app/src/pages/session/use-session-scroll-dock.ts`:
- Around line 172-179: The diagnostics hook input.onDockHeightChange is
currently invoked directly and can throw, breaking resize/scroll recovery; wrap
the invocation of input.onDockHeightChange (inside the dockHeight !==
previousDockHeight branch) in a try/catch so any exception is caught and does
not propagate, and log the error (e.g., via console.error or existing logger)
while keeping the conditional call (input.onDockHeightChange?.(...)) behavior
intact so diagnostics failures are fail-open.
In `@packages/desktop-electron/src/main/feedback.ts`:
- Around line 221-226: Wrap the await deps.rendererDiagnostics(context) call in
a timeout guard (e.g., Promise.race) so it cannot hang indefinitely; if the
timeout elapses, treat it as a handled error by calling
deps.onHandledError?.("renderer diagnostics slice timed out", timeoutError) and
set rendererDiagnostics = emptyRendererDiagnosticsSlice("write_failed", new
Date(generatedAt)) — mirror the fail-open behavior used for session export; use
a short configurable timeout constant (e.g., TIMEOUT_MS) and reference
deps.rendererDiagnostics, deps.onHandledError, emptyRendererDiagnosticsSlice,
and generatedAt when implementing.
In `@packages/desktop-electron/src/main/renderer-diagnostics.ts`:
- Around line 446-468: The record() function still writes events to disk even
when options.disabled is true; add an early check at the top of record (before
sanitizing/enqueueing) to honor options.disabled and immediately return { ok:
false, reason: "disabled" } (matching slice() behavior) so no further processing
(sanitizeRendererDiagnosticEvent, enqueueWrite, appendFile, flushRetention)
occurs when disabled.
---
Outside diff comments:
In `@packages/desktop-electron/src/main/problem-report.ts`:
- Around line 491-499: The type guard isTruncation currently omits checking
omittedRendererDiagnosticsBytes, so update isTruncation (the function checking
Payload["truncation"]) to also require
isFiniteNumber(value.omittedRendererDiagnosticsBytes); ensure the guard returns
false if that property is missing or non-numeric by adding it alongside the
other isFiniteNumber checks so parseProblemReportPayload can't produce a Payload
with an invalid truncation field.
---
Nitpick comments:
In `@packages/app/src/app.tsx`:
- Around line 50-51: The Window.api type is missing the exportDiagnosticsLog
method causing app-side typing drift; update the Window.api interface (the same
area that currently declares emitRendererDiagnostic) to also include
exportDiagnosticsLog with the same signature as the preload API—use the existing
RendererDiagnosticInput type if appropriate and ensure the method name and
parameter/return types match the exported preload surface so both
emitRendererDiagnostic and exportDiagnosticsLog are declared consistently.
In `@packages/desktop-electron/src/main/ipc-window-config.test.ts`:
- Around line 20-26: The test is brittle because it asserts string substrings
from src/main/ipc.ts instead of verifying the bootstrap wiring; update
packages/desktop-electron/src/main/ipc-window-config.test.ts to assert that the
bootstrap entry (src/main/index.ts) invokes the module-level registration
function(s) (e.g., registerRendererDiagnosticsIpc or the generic register*Ipc
exports) rather than checking for specific channel strings; modify the test to
import the bootstrap (index.ts) and spy/mock the exported register*Ipc
function(s) from the diagnostics module (or assert that index.ts imports and
calls registerRendererDiagnosticsIpc) so the test validates integration at the
bootstrap level and not file content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fc7e3280-b9b7-4318-ad00-ed3ff5d3994d
📒 Files selected for processing (29)
packages/app/e2e/session/session-renderer-diagnostics.spec.tspackages/app/src/app.tsxpackages/app/src/components/prompt-input/submit.tspackages/app/src/context/platform.tsxpackages/app/src/context/renderer-diagnostics.test.tspackages/app/src/context/renderer-diagnostics.tspackages/app/src/desktop-api.tspackages/app/src/pages/session.tsxpackages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/use-session-scroll-dock.test.tspackages/app/src/pages/session/use-session-scroll-dock.tspackages/app/src/pages/session/use-session-timeline-interaction.tspackages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc-window-config.test.tspackages/desktop-electron/src/main/ipc.tspackages/desktop-electron/src/main/menu-labels.test.tspackages/desktop-electron/src/main/menu-labels.tspackages/desktop-electron/src/main/menu-template.test.tspackages/desktop-electron/src/main/menu-template.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/renderer-diagnostics.test.tspackages/desktop-electron/src/main/renderer-diagnostics.tspackages/desktop-electron/src/main/server-client.test.tspackages/desktop-electron/src/main/server-client.tspackages/desktop-electron/src/preload/index.tspackages/desktop-electron/src/preload/types.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/desktop-electron/src/main/renderer-diagnostics.ts`:
- Around line 266-279: parseEventLine currently returns any object-shaped data
from disk; update parseEventLine (the function returning
RendererDiagnosticEvent) to re-sanitize fields before returning: validate
event.name and level against the known allowlists (e.g., permitted event name
strings and allowed level values) and reject if not in those sets, and rebuild
the returned data object by copying only the allowed keys from the original
value.data (using your established allowlist for data fields) so no unexpected
properties are passed through to exportRendererDiagnosticsLog() or slice(); keep
all other existing shape checks and return undefined on any mismatch.
- Around line 510-517: The high-frequency throttling uses Date.now(), causing
divergence from the recorder's injected clock; change the timestamp source in
the block that computes `current` (where `key`, `current`, `previous`,
`lastHighFrequency`, and `highFrequencyIntervalMs` are used) to use the
recorder's injected time function (e.g., `options.now()` or the same `now` used
elsewhere) instead of `Date.now()` so rate limiting and retention follow the
same clock as the rest of the recorder.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a37a79ff-b098-402b-92c8-1d8837496a2b
📒 Files selected for processing (11)
packages/app/e2e/session/session-renderer-diagnostics.spec.tspackages/app/src/app.tsxpackages/app/src/components/prompt-input/submit.tspackages/app/src/context/renderer-diagnostics.tspackages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/use-session-scroll-dock.tspackages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/renderer-diagnostics.test.tspackages/desktop-electron/src/main/renderer-diagnostics.ts
✅ Files skipped from review due to trivial changes (3)
- packages/app/src/app.tsx
- packages/desktop-electron/src/main/renderer-diagnostics.test.ts
- packages/app/src/components/prompt-input/submit.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/app/src/pages/session/use-session-scroll-dock.ts
- packages/desktop-electron/src/main/feedback.ts
- packages/app/e2e/session/session-renderer-diagnostics.spec.ts
- packages/desktop-electron/src/main/feedback.test.ts
- packages/desktop-electron/src/main/problem-report.ts
- packages/app/src/context/renderer-diagnostics.ts
Astro-Han
left a comment
There was a problem hiding this comment.
PR #392 Code Review: Renderer Diagnostics Observability
Overall this is a high-quality PR with clear architecture, comprehensive tests, and solid fail-open design. The main concerns are around type safety boundaries, performance details, and code organization.
P0 — Blocking (must fix)
eventMatchesSessionaccessesdata.*_session_idfields not in the allowlist for non-transition events — silent logic failure.PerformanceObserveruses deprecatedentryTypesparameter — will trigger warnings in newer Chromium.flushRetentionhas a race condition when called concurrently (not serialized throughenqueueWrite).
P1 — Serious (strongly recommended)
stringFieldURL regex is overly broad and drops legitimateendpoint_kindvalues.capEventshas O(n²) JSON stringify overhead during truncation loops.session.timeline.visibleeffect dependency tracking creates large strings on every render.requestAnimationFrameinonScrollmay fire after unmount.rendererDiagnosticsSliceIPC handler declares unuseddirectoryparameter.- Truncation loop can delete protected events if all remaining are protected.
P2 — Moderate (recommended)
highFrequencyIntervalMsdefault 250ms may miss fast scroll jumps.- FPS calculation is inaccurate when tab is backgrounded.
emitRendererDiagnosticsilently swallows all errors even in dev.- Menu placement of Export Diagnostics is inconsistent with conditional Report Problem.
SliceInput.nowis required in type but omitted in public API.route_readyandvisible_readyalways have identical values.
P3 — Minor / Style
16–24. Code organization (566-line file), test splitting, indentation consistency, threshold tuning, unit assumptions, and type safety nits.
See inline comments for specific locations and suggestions.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/app/src/pages/session.tsx`:
- Around line 137-176: The emitRendererDiagnostic call inside the
createEffect/on callbacks can reject and cause unhandled promise rejections;
make these diagnostics "fail open" by ensuring the returned promise is
swallowed—attach a .catch(() => {}) (or equivalent try/catch around an awaited
call) to the emitRendererDiagnostic invocation in both places so any rejection
is suppressed; locate the calls to emitRendererDiagnostic inside the
createEffect/on callback(s) (symbols: createEffect, on, emitRendererDiagnostic,
timelineMessageMetrics) and add the catch handler to silently ignore errors.
In `@packages/app/src/pages/session/use-session-refresh-effects.ts`:
- Around line 26-63: syncSessionWithDiagnostics currently re-reads
input.timelineSessionID() inside the .then/.catch, causing end/failed events to
use a different session id if navigation occurs; capture the start-time IDs up
front (e.g., const startedVisibleId = input.timelineSessionID() and any other
session id used at start) and use those variables for
visible_session_id/timeline_session_id in the emitted *_end and *_failed events
instead of calling input.timelineSessionID() again; apply the same change to
syncTodoWithDiagnostics (and any similar helpers) so start and completion/failed
diagnostics consistently reference the same session ids.
- Around line 22-24: The emitRefresh helper currently calls
input.emitRendererDiagnostic?.(event) without handling rejected promises so
failures produce unhandled rejections; update emitRefresh to call
input.emitRendererDiagnostic?.(event) and explicitly handle the returned Promise
(e.g., chain .catch(...) or await inside a try/catch) to swallow or log errors
from the IPC/logger path; reference emitRefresh, input.emitRendererDiagnostic
and RendererDiagnosticInput when locating the call and ensure any caught error
is handled (logged via existing logger) or ignored so refreshes fail open.
In `@packages/desktop-electron/src/main/ipc.ts`:
- Around line 361-370: The call to deps.rendererDiagnosticsSlice(...) in the
export-session path can hang; wrap that await in the same timeout guard used by
feedback code so the slice fetch is bounded (e.g., Promise.race with a timeout
promise or reuse the existing timeout helper), and on timeout or rejection fall
back to rendererDiagnostics = emptyRendererDiagnosticsSlice("write_failed", new
Date()); adjust the surrounding try/catch so timeouts are handled the same way
feedback.ts does and ensure you reference the same
SESSION_EXPORT_RENDERER_DIAGNOSTICS_MAX_BYTES and windowID parameters when
invoking rendererDiagnosticsSlice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 216f7e1a-1198-4780-a754-08fd3af67e36
📒 Files selected for processing (12)
packages/app/src/context/renderer-diagnostics.test.tspackages/app/src/context/renderer-diagnostics.tspackages/app/src/pages/session.tsxpackages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/use-session-refresh-effects.tspackages/desktop-electron/src/main/feedback.test.tspackages/desktop-electron/src/main/feedback.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc.tspackages/desktop-electron/src/main/renderer-diagnostics.test.tspackages/desktop-electron/src/main/renderer-diagnostics.tspackages/desktop-electron/src/renderer/index.tsx
✅ Files skipped from review due to trivial changes (4)
- packages/desktop-electron/src/main/feedback.test.ts
- packages/app/src/pages/session/message-timeline.tsx
- packages/desktop-electron/src/main/renderer-diagnostics.test.ts
- packages/desktop-electron/src/main/renderer-diagnostics.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/app/src/context/renderer-diagnostics.test.ts
- packages/desktop-electron/src/main/index.ts
- packages/app/src/context/renderer-diagnostics.ts
09b17ac to
ec54e50
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/desktop-electron/src/main/index.ts`:
- Around line 585-587: The adapter currently ignores the IPC-provided window
context when calling reportProblem; update the exported adapter entry (the arrow
for reportProblem) to accept and forward the second argument so the call becomes
something like reportProblem(input, context) instead of only
reportProblem(input); ensure the symbol referenced in the diff (reportProblem)
and the IPC caller in packages/desktop-electron/src/main/ipc.ts
(deps.reportProblem(input, { windowID: win?.id })) are aligned and update any
TypeScript types/signature for reportProblem to accept the optional context
parameter so windowID is preserved through to the actual report handler.
In `@packages/desktop-electron/src/main/renderer-diagnostics.ts`:
- Around line 566-575: The code sets a module-scoped flag writeFailed in
record() on any write error and then causes slice() to always return
emptyRendererDiagnosticsSlice("write_failed", ...) even after the underlying log
becomes readable again; change this so writeFailed is only a transient failure
indicator: clear writeFailed when a subsequent readEventReport() or a successful
record() completes (e.g., set writeFailed = false after readEventReport()
succeeds or after successful writes), and ensure slice() only returns the
"write_failed" empty slice when the most recent I/O attempt actually failed
rather than relying on a permanent flag; update references in record(), slice(),
and the readEventReport() call to implement this behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 26367ebf-bd7f-44a2-a605-158e39651e01
📒 Files selected for processing (10)
packages/app/src/context/renderer-diagnostics.tspackages/app/src/pages/session.tsxpackages/app/src/pages/session/message-timeline.tsxpackages/app/src/pages/session/use-session-refresh-effects.tspackages/app/src/pages/session/use-session-timeline-data.tspackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/ipc.tspackages/desktop-electron/src/main/problem-report.test.tspackages/desktop-electron/src/main/problem-report.tspackages/desktop-electron/src/main/renderer-diagnostics.ts
✅ Files skipped from review due to trivial changes (1)
- packages/desktop-electron/src/main/problem-report.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/app/src/pages/session/message-timeline.tsx
- packages/desktop-electron/src/main/problem-report.ts
- packages/app/src/context/renderer-diagnostics.ts
- packages/app/src/pages/session.tsx
- packages/app/src/pages/session/use-session-refresh-effects.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/desktop-electron/src/main/renderer-diagnostics.test.ts (1)
437-439: ⚡ Quick winUse allowlisted incident field names in this fixture.
For
incident.session_timeline_remount, Line 438 currently usesmounts/unmounts, so the sanitizer can drop them silently and the test still passes without validating incident payload retention.💡 Suggested diff
const second = sanitizeRendererDiagnosticEvent( { name: "incident.session_timeline_remount", - data: { mounts: 2, unmounts: 1 }, + data: { timeline_mount_count: 2, timeline_unmount_count: 1 }, }, { appLaunchID: "launch_1", windowID: 1, now: () => new Date("2026-05-02T10:01:00.000Z") }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-electron/src/main/renderer-diagnostics.test.ts` around lines 437 - 439, The fixture for the incident named "incident.session_timeline_remount" uses non-allowlisted fields "mounts" and "unmounts" which the sanitizer will drop; update the object in renderer-diagnostics.test.ts (the fixture with name "incident.session_timeline_remount") to use the sanitizer's allowlisted incident field names instead—replace "mounts"/"unmounts" with the exact allowed keys from the incident field allowlist used by the sanitizer (consult the sanitizer's allowlist and use those key names) so the test validates payload retention correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/desktop-electron/src/main/renderer-diagnostics.test.ts`:
- Around line 437-439: The fixture for the incident named
"incident.session_timeline_remount" uses non-allowlisted fields "mounts" and
"unmounts" which the sanitizer will drop; update the object in
renderer-diagnostics.test.ts (the fixture with name
"incident.session_timeline_remount") to use the sanitizer's allowlisted incident
field names instead—replace "mounts"/"unmounts" with the exact allowed keys from
the incident field allowlist used by the sanitizer (consult the sanitizer's
allowlist and use those key names) so the test validates payload retention
correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e8b9501d-23c9-44e7-b843-c37e9b65eb7f
📒 Files selected for processing (4)
packages/app/src/pages/session.tsxpackages/desktop-electron/src/main/index.tspackages/desktop-electron/src/main/renderer-diagnostics.test.tspackages/desktop-electron/src/main/renderer-diagnostics.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-electron/src/main/index.ts
- packages/desktop-electron/src/main/renderer-diagnostics.ts
Summary
Adds a default-on renderer diagnostics pipeline for session rendering issues:
Why
Issue #389 needs better observability for recurring session jump-to-top and render flicker bugs. The goal is to stop relying only on Computer Use and visual inspection by giving manually shared problem reports/session exports enough structured renderer evidence to diagnose these failures.
Related Issue
Closes #389
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
Please focus on:
Risk Notes
Export Diagnostics Log.../导出诊断日志....How To Verify
Latest local verification on head
70f8cfc79:Screenshots or Recordings
Not captured. The visible UI change is a native menu item and native save dialog title; coverage is via menu/i18n tests. Manual visual follow-up is still useful on macOS/Windows native menus.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
New Features
Enhancements
Tests